Skip to content

fix(index): preserve persisted coverage summaries - #1326

Open
Enferlain wants to merge 1 commit into
DeusData:mainfrom
Enferlain:fix/1287-persisted-coverage-summary
Open

fix(index): preserve persisted coverage summaries#1326
Enferlain wants to merge 1 commit into
DeusData:mainfrom
Enferlain:fix/1287-persisted-coverage-summary

Conversation

@Enferlain

Copy link
Copy Markdown

What does this PR do?

Fixes #1287.

The index response derived parse_partial_count only from errors produced by the current pipeline run. Incremental no-op and artifact-backed paths could consequently report zero even when persisted coverage rows still recorded parse gaps.

This change loads the authoritative coverage summary from the stored graph when composing the response, with a run-local fallback if that read fails. By-design exclusions remain reported separately. A regression test covers an unchanged incremental run retaining its persisted partial-parse status.

Verification

  • make -f Makefile.cbm test-focused TEST_SUITES=index_resilience — 6 passed
  • make -f Makefile.cbm test-focused TEST_SUITES='cypher extraction registry pipeline mcp index_resilience' — 884 passed, 2 skipped

Checklist

  • Every commit is signed off (git commit -s) and follows the Contributor License Agreement
  • Full test suite passes locally (make -f Makefile.cbm test) — focused sanitized suites passed
  • Lint passes (make -f Makefile.cbm lint-ci) — not run locally
  • New behavior is covered by a regression test that would fail without this fix

Signed-off-by: imi <hoshinoimi@gmail.com>
@DeusData

Copy link
Copy Markdown
Owner

Reviewed — this is correct and merge-quality, and neither of its two failing checks is your fault. It is the cleanest of your four PRs.

The loss is real on main, and I verified it rather than taking the issue's word for it. build_index_success_response derives skipped and parse_partial from the current run's file_errors only (mcp.c:6969-6970). So an incremental run that never revisits a previously-flagged file reports parse_partial_count: 0, while index_status — which reads the persisted coverage rows — says 2. That is exactly #1287's repro: two surfaces disagreeing about the same fact, with the more prominent one wrong.

Your fix reads the authoritative persisted coverage, filters out the by-design not_indexed_dir/not_indexed_file rows (which have their own not_indexed_files surface, so counting them here would double-report), maps the rest into the existing cbm_file_error_t view, and feeds the same two emitters. Reusing the emitters rather than writing a parallel formatter is the right call — the two paths cannot drift.

I checked the memory handling specifically, since it is the kind of change where a borrowed pointer escapes: the borrowed rows[i] strings are copied into the JSON document via yyjson_mut_obj_add_strcpy before free(failures) and cbm_store_free_coverage, calloc failure is checked with correct cleanup, and the const-cast is read-only plumbing. No use-after-free. The fallback to run-local errors when the coverage read fails is the right degradation.

On the two red checks — both non-genuine:

  • test / test-unix (macos-15-intel) failed on lock_registry_large_queue_parks_non_head_waiters and lock_registry_absolute_deadline_survives_repeated_wakes — daemon lock-registry threading tests that wait on a 500 ms window for waiters to park, on the slowest runner we have. Your diff touches only src/mcp/mcp.c response assembly and one test file. The decisive evidence: the identical leg passed on your sibling PR fix(cypher): scan all unlabeled query candidates #1323, which shares your base and has byte-identical lock-registry code. So it is a scheduler-timing flake, not your change. I have flagged those two tests separately — a bounded-transient-window wait is exactly the flaky class we are supposed to rebuild deterministic.
  • ci-ok is just the aggregator of that.

Everything else is green, including codeql-gate in 5m22s — which incidentally helped prove that the gate timeouts on your #1323 and #1324 are gate nondeterminism rather than anything in those PRs.

So: may we merge this as shown? It is still marked draft, which is the only thing holding it. If you were planning further changes, say so and I will wait. If draft is simply left over, mark it ready — or say the word here — and I will re-run CI and merge.

For context on your other three: #1325 also came back MERGE with no defects and I have asked the same question there. #1323 is a sound fix that needs a rebase over #1173/#1176 plus a maintainer decision on full-scan semantics. #1324 has a real recall regression that our smoke suite caught — details on that thread.

@Enferlain
Enferlain marked this pull request as ready for review July 31, 2026 18:12
@Enferlain
Enferlain requested a review from DeusData as a code owner July 31, 2026 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

index_repository reports parse_partial_count: 0 after incremental/artifact reload despite persisted coverage gaps

2 participants